Skip to content

Conversation

Noratrieb
Copy link
Member

This lint detects calls to a &self-taking as_ptr method, where the result is then immediately cast to a *mut T. Code like this is probably invalid, as that pointer will not have write permissions, and *mut T is usually used to write through.

Examples of broken code with this pattern:
https://miri.saethlin.dev/ub?crate=lol_alloc&version=0.1.3
https://miri.saethlin.dev/ub?crate=sophon-wasm&version=0.19.0
https://miri.saethlin.dev/ub?crate=polars-core&version=0.24.2
https://miri.saethlin.dev/ub?crate=ach-cell&version=0.1.17

changelog: Add [as_ptr_cast_mut]

@rust-highfive
Copy link

r? @dswij

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 1, 2022
This lint detects calls to a `&self`-taking `as_ptr` method, where
the result is then immediately cast to a `*mut T`. Code like this
is probably invalid, as that pointer will not have write permissions,
and `*mut T` is usually used to write through.
/// Checks for the result of a `&self`-taking `as_ptr` being cast to a mutable pointer
///
/// ### Why is this bad?
/// Since `as_ptr` took a `&self`, the pointer won't have write permissions, making it
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't is a bit too strong. This is not a dangerous pattern if the pointer points into an UnsafeCell.

I don't know how technically correct or detailed clippy docs are expected to be, but this should not be so absolute.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a unless interior mutability is used which makes this correct, but it doesn't sound great in my opinion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps

    /// Since `as_ptr` takes a `&self`, the pointer won't have write permissions unless interior
    /// mutability is used. If this cast to `*mut` exists to enable mutation through this pointer,
    /// that mutation is likely to be UB.

Copy link
Member

@dswij dswij left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM. I think the wording is ok now. If the wording is indeed too confusing, we can improve it in the future.

Comment on lines +25 to +26
// `as_mut_ptr` might not exist
let applicability = Applicability::MaybeIncorrect;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can check if it implements as_mut_ptr for the suggestion, but this can be a future improvement

@dswij
Copy link
Member

dswij commented Oct 11, 2022

Thanks for this @Nilstrieb!

@bors r+

@bors
Copy link
Contributor

bors commented Oct 11, 2022

📌 Commit 2b944d0 has been approved by dswij

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Oct 11, 2022

⌛ Testing commit 2b944d0 with merge 8e87d39...

@bors
Copy link
Contributor

bors commented Oct 11, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: dswij
Pushing 8e87d39 to master...

@bors bors merged commit 8e87d39 into rust-lang:master Oct 11, 2022
@Noratrieb Noratrieb deleted the as-ptr-cast-mut branch October 11, 2022 08:39
@bors bors mentioned this pull request Oct 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants